-
Notifications
You must be signed in to change notification settings - Fork 123
style: use dollar variables (getting links) #1844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Preview for this PR was built for commit |
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
Comment @cursor review
or bugbot run
to trigger another review on this PR
const data = []; | ||
$(".product-item").each((i, element) => { | ||
const productItem = $(element); | ||
const $items = $(".product-item").map((i, element) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const $items = $(".product-item").map((i, element) => { | |
const data = $(".product-item").toArray().map((element) => { |
Adding the toArray
instruction, you have a "regular" map method, that returns regular objects instead of Cheerio objects (less confusing), and you can remove the const data = $items.get();
later. Moreover, the i
index was unused.
}); | ||
const data = $items.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const data = $items.get(); |
const productItem = $(element); | ||
const item = parseProduct(productItem); | ||
data.push(item); | ||
const $items = $(".product-item").map((i, element) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const $items = $(".product-item").map((i, element) => { | |
const data = $(".product-item").toArray().map((element) => { |
}); | ||
const data = $items.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const data = $items.get(); |
const data = [] | ||
$(".product-item").each((i, element) => { | ||
const productItem = $(element); | ||
const $items = $(".product-item").map((i, element) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const $items = $(".product-item").map((i, element) => { | |
const data = $(".product-item").toArray().map((element) => { |
}); | ||
const data = $items.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const data = $items.get(); |
@gullmar Thank you! 🙇♂️ As I mentioned in the PR description, these changes still use |
I'm sorry, because it's a bit confusing and it's easy to overlook the note I made 🫠 |
As I progressed with apify#1584 I felt the code examples were starting to be more and more complex. Then I remembered that when I was young, us jQuery folks used to lean towards a naming convention where variables holding jQuery selections were prefixed with $. I changed the code examples in all lessons to adhere to this as I feel it makes them more readable and less cluttered. ----- ℹ️ The changes still use `$.map` and `$.each`, because they were made prior to the facb3c0 commit. It's gonna happen, but not yet.
As I progressed with #1584 I felt the code examples were starting to be more and more complex. Then I remembered that when I was young, us jQuery folks used to lean towards a naming convention where variables holding jQuery selections were prefixed with $. I changed the code examples in all lessons to adhere to this as I feel it makes them more readable and less cluttered.
ℹ️ The changes still use
$.map
and$.each
, because they were made prior to the facb3c0 commit. It's gonna happen, but not yet.